#11681 - Enable -Wshadow-field and investigate actual bugs it finds by jmarrec · Pull Request #11684 · NatLabRockies/EnergyPlus

jmarrec · 2026-07-10T14:37:15Z

Pull request overview

Fixes Enable Wshadow-field and investigate actual bugs it finds #11681
This builds upon Fix #11680 - Completely Remove the use of link_options / add_compile_options in third_party #11683 - review and merge that one first
- To review right now, exclude commits from that PR: https://github.com/NatLabRockies/EnergyPlus/pull/11684/changes/25e3884443bec67c075ae9c36d2405aba13b31ab..HEAD

Description of the purpose of this PR

Enable -Wshadow-field on clang and fixup the warnings. Found a few fixable bugs.

Found another one that will warrant another issue in HeatPumpAirToWater, for now I restored the behavior to the current so I wouldn't produce diff, but it seems that there are more than a few issues.

Pull Request Author

Reviewer

jmarrec · 2026-07-10T14:38:33Z

    target_compile_options(project_options INTERFACE $<$<CONFIG:RelWithDebInfo>:-UNDEBUG>)
    target_compile_options(project_fp_options INTERFACE -ffp-contract=off) # Disable fused-floating point operations (default is fast)
  elseif("${CMAKE_CXX_COMPILER_ID}" STREQUAL "Clang" OR "${CMAKE_CXX_COMPILER_ID}" STREQUAL "AppleClang")
+    target_compile_options(project_warnings INTERFACE -Wshadow-field) # Equivalent to MSVC's C4458 (declaration of 'identifier' hides class member); narrower than -Wshadow


New flag on clang. Like I said, -Wshadow is way too noisy

jmarrec · 2026-07-10T14:40:03Z

    };

-    struct SteamBaseboardDesignData : SteamBaseboardParams
+    struct SteamBaseboardDesignData


Wshadow revealed a big inheritance mistake in SteamBaseboardRadiator

So there’s a ZoneHVAC:Baseboard:RadiantConvective:Steam and a ZoneHVAC:Baseboard:RadiantConvective:Steam:Design objects

The C++ SteamBaseboardParams instance has an int index to find the SteamBaseboardDesignData...

But the SteamBaseboardDesignData inherits from SteamBaseboardParams, so it carries EVERY field from SteamBaseboardParams, so about 656 bytes for no reason!

It shouldn't inherit at all!

Good catch.

jmarrec · 2026-07-10T14:40:26Z

    };

-    struct HWBaseboardDesignData : HWBaseboardParams
+    struct HWBaseboardDesignData


Same issue as the Steam counterpart

jmarrec · 2026-07-10T14:41:07Z


        // logic flags
-        bool oneTimeInitFlag = true;
+        bool oneTimeInitFlagPLHP = true;


oneTimeInitFlag is defined in PlantComponent, which is inherits, but we need that flag here as it's used to call SetupOutputVariables

jmarrec · 2026-07-10T14:43:17Z

+        std::array<int, maxNumSpeeds + 1> capFuncTempCurveIndices = {};
+        std::array<int, maxNumSpeeds + 1> powerRatioFuncTempCurveIndices = {};
+        std::array<int, maxNumSpeeds + 1> powerRatioFuncPLRCurveIndices = {};


EIRPlantLoopHeatPump defines int indices

struct HeatPumpAirToWater : public EIRPlantLoopHeatPump would redefine it: it shadows with a different type. Not great.

Ideally we add a different Base but this was too much change for not so much gains, so I just defined a different one.

Yeah, that's fine, too.

jmarrec · 2026-07-10T14:45:12Z

+        // Deliberately NOT named companionHeatPumpCoil: the base class's own EIRPlantLoopHeatPump::companionHeatPumpCoil
+        // must stay null for HeatPumpAirToWater objects, because EIRPlantLoopHeatPump::sizeLoadSide() (and other base
+        // sizing methods) branch on it being non-null to size off a companion coil using logic that was written for,
+        // and only checks for, the plain PlantLoopHeatPump:EIR:Heating/Cooling pair (DataPlant::PlantEquipmentType::
+        // HeatPumpEIRHeating/Cooling), not HeatPumpAirToWater. This member is used instead by
+        // HeatPumpAirToWater::pairUpCompanionCoils() and HeatPumpAirToWater::calcOpMode().
+        HeatPumpAirToWater *companionAWHPCoil = nullptr;


This is a hard bug. Here I'm voluntarily restoring the behavior to avoid a diff.

But there's a problem in there.

Needs further investigation (separate, pre-existing issue, not caused by this branch).

HeatPumpAirToWater::sizeLoadSide() is not virtual, and its only would-be caller (EIRPlantLoopHeatPump::onInitLoopEquip) invokes this->sizeLoadSide(state) from within the base class's own scope, so that call always statically resolves to EIRPlantLoopHeatPump::sizeLoadSide,
never the derived override.
Confirmed no production or test code calls sizeLoadSide() through a HeatPumpAirToWater*-typed pointer either, so HeatPumpAirToWater::sizeLoadSide() (and its referenceCapacityOneUnit
recompute) is effictively dead code today. Left a TODO comment at the definition;

This likely needs a real fix (e.g. making sizeLoadSide virtual, or overriding onInitLoopEquip in HeatPumpAirToWater) but that's a behavior (and diff-producing) change out of scope for this PR.

Filed Inheritance and shadowing issues in HeatPumpAirToWater #11685

@rraustad FYI

The AWHP sizeLoadSide function was added here but I don't think it's used anywhere. As @jmarrec says, it's the EIRPlantLoopHeatPump::sizeLoadSide that gets called to size the plant HP. referenceCapacityOneUnit is used only internally to this model and initialized here in AWHP getInput. I suspect that this is not working as expected (i.e., if ratedCapacity is autosized then the value of referenceCapacityOneUnit is also autosize).

if (thisAWHP.ratedCapacity[thisAWHP.numSpeeds - 1] == DataSizing::AutoSize) { thisAWHP.referenceCapacityWasAutoSized = true; } thisAWHP.referenceCapacityOneUnit = thisAWHP.ratedCapacity[thisAWHP.numSpeeds - 1]; thisAWHP.referenceCapacity = thisAWHP.referenceCapacityOneUnit * thisAWHP.heatPumpMultiplier;

I have a personal requirement of only using autosized inputs in new example files for this very reason, to test all pertinent code.

The object used in PlantLoopHeatPump_EIR_AirSource_and_AWHP.idf:

HeatPump:AirToWater, test_AWHP, !- Name 20000, !- Rated Heating Capacity at Speed 1 {W} 40000, !- Rated Heating Capacity at Speed 2 {W} HeatPump:AirToWater, N25, \field Rated Heating Capacity at Speed 1 \autosizable \default autosize N27, \field Rated Heating Capacity at Speed 2 \autosizable \default autosize

And then used like this in calcOpMode, which would not work well if these input fields are autosized.

auto availableCapacityOneUnit = this->referenceCapacityOneUnit * capacityModifierFuncTemp;

The test would be to autosize these 2 inputs and watch for smoke.

Thanks for documenting it. This can be treated separately.

jmarrec · 2026-07-10T14:45:59Z

+// TODO: sizeLoadSide() is not virtual, and the only production call site (EIRPlantLoopHeatPump::onInitLoopEquip,
+// via `this->sizeLoadSide(state);`) is compiled in the base class's own scope, so it always statically resolves to
+// EIRPlantLoopHeatPump::sizeLoadSide. This override is therefore unreachable dead code today; the
+// referenceCapacityOneUnit recompute below never runs. Pre-existing issue, unrelated to the Wshadow-field cleanup.
 void HeatPumpAirToWater::sizeLoadSide(EnergyPlusData &state)
 {
    EIRPlantLoopHeatPump::sizeLoadSide(state);
    this->referenceCapacityOneUnit = this->referenceCapacity / this->heatPumpMultiplier;
 }


This code is effectively dead currently, which I suppose isn't the intent given this function does post-calculations...

EnergyPlus/src/EnergyPlus/PlantLoopHeatPumpEIR.cc

Line 1275 in 5f5c37c

if (this->companionHeatPumpCoil != nullptr) {

In EIRPlantLoopHeatPump::sizeLoadSide, the companionHeatingCoil was ALWAYS nullptr for HeatPumpAirToWater (and it is still, but it's more explicit now).

HeatPumpAirToWater would initialize its own shadowing copy of companionHeatPumpCoil and leave the base one unitialized.

And it's the Base method that's called so in there companionHeatPumpCoil is definitely the Base's, not the Derived

I have a not so MCVE at https://compiler-explorer.com/z/3vYjEj1oY that shows it, and a more MCVE (less complete) at https://gcc.godbolt.org/z/YrMY9P3q8

jmarrec · 2026-07-10T14:51:03Z

        Real64 Temperature_PrevIteration = 0.0; // C
        Real64 Temperature_PrevTimeStep = 0.0;  // C
        Real64 Beta = 0.0;                      // K/W
-        BaseThermalPropertySet Properties;


Removed BaseThermalPropertySet Properties; from BaseCell, and instead gave each of its three derived structs its own appropriately-typed Properties member directly:

RadialCellInformation → BaseThermalPropertySet Properties; (new, explicit)

CartesianCell → BaseThermalPropertySet Properties; (new, explicit)

FluidCellInformation → kept its existing ExtendedFluidProperties Properties; (unchanged)

Why: FluidCellInformation was redeclaring Properties with the wider ExtendedFluidProperties type (adds Viscosity/Prandtl) to shadow the inherited BaseThermalPropertySet Properties from BaseCell — needed so that bulk-assignments like cell.PipeCellData.Fluid.Properties = thisCircuit->CurFluidPropertySet; copy the full extended struct instead of object-slicing it. This triggered a -Wshadow-field warning, and — since C++ field-hiding isn't virtual dispatch — meant every FluidCellInformation instance carried a second, entirely dead BaseThermalPropertySet subobject inherited from BaseCell that was never read or written.

Since BaseCell itself is never instantiated or referenced directly (only used as a base for these three structs), moving Properties down into each derived struct removes the shadowing entirely and eliminates the wasted subobject, with no behavior change — RadialCellInformation and CartesianCell still get the same BaseThermalPropertySet Properties they had before, just declared locally instead of inherited

That's also a good catch.

/Users/julien/Software/Others/EnergyPlus/third_party/btwxt/include/btwxt/logging.h:33:52: error: parameter 'message' shadows member inherited from type 'CourierrException' [-Werror,-Wshadow-field] 33 | explicit BtwxtException(const std::string &message, Courierr::Courierr &logger) | ^ /Users/julien/Software/Others/EnergyPlus/third_party/btwxt/vendor/courierr/include/courierr/courierr.h:66:17: note: declared here 66 | std::string message; | ^ In file included from /Users/julien/Software/Others/EnergyPlus/src/EnergyPlus/api/state.cc:49: In file included from /Users/julien/Software/Others/EnergyPlus/src/EnergyPlus/Data/CommonIncludes.hh:266: In file included from /Users/julien/Software/Others/EnergyPlus/src/EnergyPlus/SolarShading.hh:61: In file included from /Users/julien/Software/Others/EnergyPlus/third_party/penumbra/include/penumbra/penumbra.h:16: /Users/julien/Software/Others/EnergyPlus/third_party/penumbra/include/penumbra/logging.h:40:49: error: parameter 'message' shadows member inherited from type 'CourierrException' [-Werror,-Wshadow-field] 40 | explicit PenumbraException(const std::string &message, Courierr::Courierr &logger) | ^ /Users/julien/Software/Others/EnergyPlus/third_party/btwxt/vendor/courierr/include/courierr/courierr.h:66:17: note: declared here 66 | std::string message; 51ba69

Removed `BaseThermalPropertySet Properties;` from `BaseCell`, and instead gave each of its three derived structs its own appropriately-typed `Properties` member directly: - `RadialCellInformation` → `BaseThermalPropertySet Properties;` (new, explicit) - `CartesianCell` → `BaseThermalPropertySet Properties;` (new, explicit) - `FluidCellInformation` → kept its existing `ExtendedFluidProperties Properties;` (unchanged) **Why:** `FluidCellInformation` was redeclaring `Properties` with the wider `ExtendedFluidProperties` type (adds `Viscosity`/`Prandtl`) to shadow the inherited `BaseThermalPropertySet Properties` from `BaseCell` — needed so that bulk-assignments like `cell.PipeCellData.Fluid.Properties = thisCircuit->CurFluidPropertySet;` copy the full extended struct instead of object-slicing it. This triggered a `-Wshadow-field` warning, and — since C++ field-hiding isn't virtual dispatch — meant every `FluidCellInformation` instance carried a second, entirely dead `BaseThermalPropertySet` subobject inherited from `BaseCell` that was never read or written. Since `BaseCell` itself is never instantiated or referenced directly (only used as a base for these three structs), moving `Properties` down into each derived struct removes the shadowing entirely and eliminates the wasted subobject, with no behavior change — `RadialCellInformation` and `CartesianCell` still get the same `BaseThermalPropertySet Properties` they had before, just declared locally instead of inherited.

So there’s a ZoneHVAC:Baseboard:RadiantConvective:Steam and a ZoneHVAC:Baseboard:RadiantConvective:Steam:Design objects The C++ `SteamBaseboardParams` instance has an `int` index to find the `SteamBaseboardDesignData`... But the `SteamBaseboardDesignData` inherits from `SteamBaseboardParams`, so it carries EVERY field from SteamBaseboardParams, so about 656 bytes for no reason! It shouldn't inherit at all!

… inheriting Here HWBaseboardParams::HeatingCapMethod/ScaledHeatingCapacity are genuinely used as per-instance cached copies (set once from the design object, read every timestep), so those stayed untouched

Removed four pure-duplicate member redeclarations that shadowed identical inherited fields for no reason — same name, same type, same default, no divergent usage found anywhere in the `.cc`: - `EIRPlantLoopHeatPump::oneTimeInitFlag` (line 209) — exact duplicate of `PlantComponent::oneTimeInitFlag` (`bool`, default `true`). Deleted; `this->oneTimeInitFlag` now resolves to the inherited one, same behavior. - `EIRFuelFiredHeatPump::flowMode` — exact duplicate of `EIRPlantLoopHeatPump::flowMode` (`DataPlant::FlowMode`, default `Invalid`). Deleted. - `EIRFuelFiredHeatPump::capModFTErrorIndex`, `eirModFTErrorIndex`, `eirModFPLRErrorIndex` — exact duplicates of the same-named `int` error-index members on `EIRPlantLoopHeatPump` (all default `0`). Deleted. In all four cases the derived class had no custom constructor initializing these differently, and every usage site accessed them polymorphically through `this->` — so removing the redeclaration doesn't change behavior, just stops the derived object from carrying (and the compiler from having to reason about) two separately-named-but-identical copies of the same state.

…en it goes to an array for HeatPumpAirToWater The base class xxxFuncYYYCurveIndex still exists but it's left untouched at int = 0, so trying to access curves(int) with it will throw when NDEBUG not defined, and having three unused ints beats creating a new derived class...

…he PlantComponent one

…nter type

… there's a EnergyPlusData* state member

…in there Fix regression in HeatPumpAirToWater sizing from companionHeatPumpCoil rename The Wshadow-field fix in 4f15f0b removed HeatPumpAirToWater's own `companionHeatPumpCoil` shadow member and had its pairUpCompanionCoils() assign into the base EIRPlantLoopHeatPump::companionHeatPumpCoil instead. That member is also read by EIRPlantLoopHeatPump::sizeLoadSide() (and other base sizing methods), whose companion-based sizing branch was written for, and only type-checks against, the plain PlantLoopHeatPump:EIR Heating/Cooling pair (DataPlant::PlantEquipmentType::HeatPumpEIRHeating/Cooling) -- it was never adapted for HeatPumpAirToWater. Before the rename, this branch was structurally unreachable for AWHP objects (the base member always stayed null for them); after the rename it started firing, changing autosized "Rated Water Volume Flow Rate in Heating Mode" for PlantLoopHeatPump_EIR_AirSource_and_AWHP.eio from 0.005 to 0.018 m3/s. Fixing it: Restore a separate, non-shadowing member (companionAWHPCoil) used only by `HeatPumpAirToWater::pairUpCompanionCoils()`/`calcOpMode()`, leaving the base member's null-for-AWHP behavior intact, matching pre-fix sizing results. Needs further investigation (separate, pre-existing issue, not caused by this branch). `HeatPumpAirToWater::sizeLoadSide()` is not virtual, and its only would-be caller (EIRPlantLoopHeatPump::onInitLoopEquip) invokes `this->sizeLoadSide(state)` from within the base class's own scope, so that call always statically resolves to `EIRPlantLoopHeatPump::sizeLoadSide`, never the derived override. Confirmed no production or test code calls `sizeLoadSide()` through a HeatPumpAirToWater*-typed pointer either, so `HeatPumpAirToWater::sizeLoadSide()` (and its referenceCapacityOneUnit recompute) is effictively dead code today. Left a TODO comment at the definition; This likely needs a real fix (e.g. making sizeLoadSide virtual, or overriding onInitLoopEquip in HeatPumpAirToWater) but that's a behavior (and diff-producing) change out of scope for this PR.

mitchute

Thanks @jmarrec. Looks good.

mitchute · 2026-08-10T18:26:31Z

    };

-    struct SteamBaseboardDesignData : SteamBaseboardParams
+    struct SteamBaseboardDesignData


Good catch.

mitchute · 2026-08-10T18:27:58Z

        Real64 Temperature_PrevIteration = 0.0; // C
        Real64 Temperature_PrevTimeStep = 0.0;  // C
        Real64 Beta = 0.0;                      // K/W
-        BaseThermalPropertySet Properties;


That's also a good catch.

mitchute · 2026-08-10T18:28:14Z

+        std::array<int, maxNumSpeeds + 1> capFuncTempCurveIndices = {};
+        std::array<int, maxNumSpeeds + 1> powerRatioFuncTempCurveIndices = {};
+        std::array<int, maxNumSpeeds + 1> powerRatioFuncPLRCurveIndices = {};


Yeah, that's fine, too.

mitchute · 2026-08-10T18:28:33Z

+        // Deliberately NOT named companionHeatPumpCoil: the base class's own EIRPlantLoopHeatPump::companionHeatPumpCoil
+        // must stay null for HeatPumpAirToWater objects, because EIRPlantLoopHeatPump::sizeLoadSide() (and other base
+        // sizing methods) branch on it being non-null to size off a companion coil using logic that was written for,
+        // and only checks for, the plain PlantLoopHeatPump:EIR:Heating/Cooling pair (DataPlant::PlantEquipmentType::
+        // HeatPumpEIRHeating/Cooling), not HeatPumpAirToWater. This member is used instead by
+        // HeatPumpAirToWater::pairUpCompanionCoils() and HeatPumpAirToWater::calcOpMode().
+        HeatPumpAirToWater *companionAWHPCoil = nullptr;


Thanks for documenting it. This can be treated separately.

jmarrec self-assigned this Jul 10, 2026

jmarrec added Defect Includes code to repair a defect in EnergyPlus Developer Issue Related to cmake, packaging, installers, or developer tooling (CI, etc) labels Jul 10, 2026

jmarrec commented Jul 10, 2026

View reviewed changes

jmarrec mentioned this pull request Jul 10, 2026

Inheritance and shadowing issues in HeatPumpAirToWater #11685

Open

jmarrec requested a review from mitchute July 10, 2026 15:03

jmarrec added 15 commits August 10, 2026 14:54

Use Wshadow-field on clang (GCC only has Wshadow which is way too noisy)

2ca312b

Fix some warnings in Windows-CalcEngine

0d11994

Fix two Wshadow warnings anyways in BCVTB

851db33

Start fixing Wshadow-field warnings

8f7f10c

HWBaseboardRadiator.hh: Same as SteamBaseboardRadiator: should NOT be…

4584390

… inheriting Here HWBaseboardParams::HeatingCapMethod/ScaledHeatingCapacity are genuinely used as per-instance cached copies (set once from the design object, read every timestep), so those stayed untouched

I need a unique oneTimeInitFlag actually here, it does things after t…

1826117

…he PlantComponent one

Last shadow issue: companionHeatPumpCoil redefined as a different poi…

2ac5bf6

…nter type

Error on Wshadow-field now

77e7c35

EnergyPlusFixture functions shouldn't take EnergyPlusData& state when…

a9bfd7a

… there's a EnergyPlusData* state member

jmarrec force-pushed the Wshadow-field branch from c823fc4 to ae83cce Compare August 10, 2026 13:19

jmarrec marked this pull request as ready for review August 10, 2026 14:47

mitchute approved these changes Aug 10, 2026

View reviewed changes

mitchute merged commit 46fa6c7 into develop Aug 10, 2026
9 of 10 checks passed

mitchute deleted the Wshadow-field branch August 10, 2026 18:32

Uh oh!

Conversation

jmarrec commented Jul 10, 2026

Pull request overview

Description of the purpose of this PR

Pull Request Author

Reviewer

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmarrec Jul 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mitchute left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jmarrec Jul 10, 2026 •

edited

Loading